I think we could also delete the IAttr class entirely and replace it with __new__ on the Intrinsic Enum, any preferences @sergisiso

I think we could also delete the IAttr class entirely and replace it with __new__ on the Intrinsic Enum, any preferences @sergisiso

I quickly tried this and decided its worse because you can't pass named arguments to __new__, meaning we'd be stuck again with the non-named Enum argument lists which are getting too long now to be readable.

I've fixed the remaining problems with the test suite. I expect there will be some coverage changes I need to look at next, notably I expect there are now unreachable sections of fparser2.py that I can remove, but I will wait to see what pycov says and go from there in case there are other sections I either miss testing or that can now be removed.

One option I leave up to the review is whether to add something to the backend to not output argument names, as we will now be very explicit and add argument names wherever possible (assuming an IntrinsicCall has been canonicalised).

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (5ee8752) to head (28899ef).

@@           Coverage Diff            @@
##           master    #3150    +/-   ##
========================================
  Coverage   99.90%   99.90%            
========================================
  Files         374      374            
  Lines       53011    53165   +154     
========================================
+ Hits        52958    53112   +154     
  Misses         53       53            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arporter @sergisiso This is ready for a first look now.

Yes, clarified.

I haven't seen the clarification commit yet but I have been looking at the code and it seems we don't do re-ordering anymore. The frontend (through create) just tries to match the interface and give names, and produces a CodeBlock if it can't. The transformations should be using the argument_by_name. The backend just omits names while they match the standard order, if they don't or there is any problem it continues with names. Is this a correct summary?

If that's the case, maybe we don't need the flag? This seems a good output style (no names while they match the order, named afterwards or in case of doubt). It also prevents the NEMO sign issue. And I am not too keen on having output style knobs. What do you think @arporter @LonelyCat124 about making that the backend behaviour (no options needed).

That said, NEMOv5 fails with a wrong arg-name that will need to be fixed:

errioipsl.psycloned.f90:36:34:

   36 |       ilv_max = MAX(ilv_max, None=plev)
      |                                  1
Error: Unknown argument 'none' at (1) to intrinsic max

Also, I am not sure canonicalise is the right word anymore. To me it implies reshaping which we don't do anymore, and without being familiar with this PR I probably wouldn't understand what it does without looking inside. A better name for the canonicalise method could be:
call.provide_all_argument_names()
or
call.provide_all_argument_names_or_fail() (since sometimes is used only for validation)

To be clear, I am not suggesting any implementation change, just renaming the method and associated comments.

It does a bit more than naming arguments: it works out which version of an intrinsic is being called. Perhaps identify... or match_interface... or something?

Also, I am not sure canonicalise is the right word anymore. To me it implies reshaping which we don't do anymore, and without being familiar with this PR I probably wouldn't understand what it does without looking inside. A better name for the canonicalise method could be: call.provide_all_argument_names() or call.provide_all_argument_names_or_fail() (since sometimes is used only for validation)

To be clear, I am not suggesting any implementation change, just renaming the method and associated comments.

I debated this previously when i stopped reshaping, but I felt like we still end up with a canonicalised (standard w.r.t argument names, but not argument order) version of the IntrinsicCall so I didn't rename it. I can rename if you feel strongly about it though. Either way I probably need to check over the docstring and docs.

If that's the case, maybe we don't need the flag? This seems a good output style (no names while they match the order, named afterwards or in case of doubt). It also prevents the NEMO sign issue. And I am not too keen on having output style knobs. What do you think @arporter @LonelyCat124 about making that the backend behaviour (no options needed).

It only removes required argument names (as I think optional ones should be left in generally).
I don't mind changing the default output style to that, but then I'd want to add a "leave all argument names" option (as I feel giving code output options when we have information is reasonable). I also feel like once the default behaviour is to remove argument names from required arguments if we can, then I struggle to see the same argument for not reordering and removing for consistency of output, though changing order at output-time does get a bit weird w.r.t transformations.

My problem with node.canonicalise is that is not obvious what it does or that is also a validation without reading documentation, and a more descriptive name would help. Also we have been using it in the frontend to mean "convert fparser constructs to the available psyir nodes", but here the concept is used slightly differently and it escapes the frontend, so I think a more specific name will be nicer.

as I feel giving code output options when we have information is reasonable

I will let you two decide about this. I don't think I care too much about the output style as far as is readable and reasonable. The previous backend options are to bypass issues. Going down to provide style options could make the backend much more complex and I am not sure this is the focus of psyclone. But if it is just this one it is not a big deal.

Yeah I'm fine to change it then. I'll look at doing that after the bug fix.

I'm also happy I think to change the default output and have an option to enable argument names separately.

I broke all the sympy stuff again when switching the default behaviour :'(

Hopefully integration works now, there was one badly defined intrinsic (capitalized argument names which docs say we shouldn't have) that I've fixed and I'm hoping was the remaining issue.

@arporter I think this is ready for another look now - I've fixed the issues discussed and the NEMO4 integration tests work - I'm waiting on the others to finish running but the first LFRic attempt had the signal 11 issue from the Nvidia compiler so I've set it rerunning.